Skip to content

Use modern techniques to delete copy constructors and assignment operators#1233

Merged
feldergast merged 1 commit intosstsimulator:develfrom
leekillough:deleted_copy_assign
Mar 8, 2025
Merged

Use modern techniques to delete copy constructors and assignment operators#1233
feldergast merged 1 commit intosstsimulator:develfrom
leekillough:deleted_copy_assign

Conversation

@leekillough
Copy link
Contributor

This uses modern methods of deleting copy constructors and copy assignment operators, with = delete.

Copy assignment operators declared void operator=() were changed to class& operator=() to become a copy assignment operator as defined by the language, so that the default one can be deleted.

For some classes, a default constructor previously explicitly declared private to prevent its use outside of the class was removed, because the declaration of the copy constructor causes the deletion of the default constructor.

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT FAIL labels Mar 6, 2025
@github-actions
Copy link

github-actions bot commented Mar 6, 2025

CLANG-FORMAT TEST - FAILED (on last commit):
Run > ./scripts/clang-format-test.sh using clang-format v12 to check formatting

@github-actions github-actions bot added AT: CMAKE-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 6, 2025
@github-actions
Copy link

github-actions bot commented Mar 6, 2025

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@leekillough leekillough force-pushed the deleted_copy_assign branch from 1774361 to 64ef1b3 Compare March 6, 2025 01:24
@github-actions github-actions bot added the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Mar 6, 2025
@github-actions
Copy link

github-actions bot commented Mar 6, 2025

CLANG-FORMAT TEST - FAILED (on last commit):
Run > ./scripts/clang-format-test.sh using clang-format v12 to check formatting

@github-actions github-actions bot removed the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Mar 6, 2025
@github-actions
Copy link

github-actions bot commented Mar 6, 2025

CMAKE-FORMAT TEST - PASSED

@leekillough leekillough force-pushed the deleted_copy_assign branch from 64ef1b3 to 1616eda Compare March 6, 2025 01:28
@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 6, 2025
@github-actions
Copy link

github-actions bot commented Mar 6, 2025

CMAKE-FORMAT TEST - PASSED

@github-actions
Copy link

github-actions bot commented Mar 6, 2025

CLANG-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed.

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

  • Build Num: 1849
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

  • Build Num: 1805
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

  • Build Num: 1804
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

  • Build Num: 803
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

  • Build Num: 656
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

  • Build Num: 610
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

  • Build Num: 372
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

  • Build Num: 228
  • Status: STARTED

Using Repos:

Repo: CORE (leekillough/sst-core)
  • Branch: deleted_copy_assign
  • SHA: 1616eda
  • Mode: TEST_REPO
Repo: SQE (sstsimulator/sst-sqe)
  • Branch: devel
  • SHA: fc3830fbe74ce06cebb6b6838c78777a6855707a
  • Mode: SUPPORT_REPO
Repo: ELEMENTS (sstsimulator/sst-elements)
  • Branch: devel
  • SHA: de2c6802a84ae1701a768d7387fc207adc867056
  • Mode: SUPPORT_REPO
Repo: MACRO (sstsimulator/sst-macro)
  • Branch: devel
  • SHA: 42e85e1689d473c65fdbcc008ce57fd53fe80865
  • Mode: SUPPORT_REPO

Pull Request Author: leekillough

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

  • Build Num: 1849
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

  • Build Num: 1805
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

  • Build Num: 1804
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

  • Build Num: 803
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

  • Build Num: 656
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

  • Build Num: 610
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

  • Build Num: 372
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

  • Build Num: 228
  • Status: PASSED

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
WARNING: NO REVIEWERS HAVE BEEN REQUESTED FOR THIS PULL REQUEST!

@sst-autotester
Copy link
Contributor

All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur...

3 similar comments
@sst-autotester
Copy link
Contributor

All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur...

@sst-autotester
Copy link
Contributor

All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur...

@sst-autotester
Copy link
Contributor

All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur...

Copy link
Member

@berquist berquist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think separately there should also be a policy on move, now that the copy constructors and assignment are explicitly deleted.

@leekillough
Copy link
Contributor Author

I think separately there should also be a policy on move, now that the copy constructors and assignment are explicitly deleted.

Or, at least, The Rule of 3, or The Rule of 5 if moving is significantly faster than copying (such as if new memory is dynamically allocated during copying but can simply be reused during moving, or if deep copies are expensive while shallow copies just move a few pointers around), or moving is allowed and copying is not, e.g. such as std::unique_ptr.

= default can be used instead of = delete if the behavior of copying, moving or assigning a class member-by-member (including direct bases) is the desired behavior. If the compiler cannot automatically generate copying/ moving/ assignment member-by-member, then explicitly declaring = default will cause a compilation error while leaving = default out will implicitly delete the method without explicitly declaring it = delete, while explicit = delete is preferred.

For C-style structs without explicit constructors, the default behavior can be relied upon without explicitly declaring constructors or assignment operators. The Rule of 3/5/0 says that if you declare one of (copy constructor, copy assignment operator, destructor), you should declare all 3, even if only = default, and if you declare one of (move constructor, move assignment operator), you should declare all 5 of the aforementioned.

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ berquist ]!

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

5 similar comments
@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@feldergast feldergast merged commit b309ec0 into sstsimulator:devel Mar 8, 2025
7 checks passed
kpgriesser pushed a commit to tactcomplabs/sst-core that referenced this pull request Mar 14, 2025
@leekillough leekillough deleted the deleted_copy_assign branch March 31, 2025 04:07
@berquist berquist added this to the SST V15.0.0 milestone Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants